Conversation
|
The default antialiasing is different now, so some differences in the baselines are expected |
| if (useLineAA && abs(fLineEdgeDist) > 0.001) | ||
| { | ||
| float dist = abs(fLineEdgeDist); | ||
| float delta = fwidth(dist); | ||
|
|
||
| // Edge transition with asymmetric smoothing | ||
| float edge0 = 1.0 - delta * 1.6; | ||
| float edge1 = 1.0 + delta * 0.6; | ||
| float t = clamp((dist - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
|
|
||
| // Smootherstep interpolation (C2 continuous) | ||
| float smootherT = t * t * t * (t * (t * 6.0 - 15.0) + 10.0); | ||
| float alpha = 1.0 - smootherT; | ||
| alpha = alpha * (1.0 + alpha * 0.03); | ||
|
|
||
| // Dual-ring multi-sampling (8 inner + 4 outer samples) | ||
| float dx = dFdx(fLineEdgeDist); | ||
| float dy = dFdy(fLineEdgeDist); | ||
|
|
||
| const float r1 = 0.3535533905932738; // Inner ring: 1/sqrt(8) | ||
| float s1 = abs(fLineEdgeDist + dx * r1); | ||
| float s2 = abs(fLineEdgeDist - dx * r1); | ||
| float s3 = abs(fLineEdgeDist + dy * r1); | ||
| float s4 = abs(fLineEdgeDist - dy * r1); | ||
| float s5 = abs(fLineEdgeDist + (dx + dy) * r1 * 0.707); | ||
| float s6 = abs(fLineEdgeDist + (dx - dy) * r1 * 0.707); | ||
| float s7 = abs(fLineEdgeDist - (dx + dy) * r1 * 0.707); | ||
| float s8 = abs(fLineEdgeDist - (dx - dy) * r1 * 0.707); | ||
|
|
||
| const float r2 = 0.5; // Outer ring | ||
| float s9 = abs(fLineEdgeDist + (dx + dy) * r2); | ||
| float s10 = abs(fLineEdgeDist + (dx - dy) * r2); | ||
| float s11 = abs(fLineEdgeDist - (dx + dy) * r2); | ||
| float s12 = abs(fLineEdgeDist - (dx - dy) * r2); | ||
|
|
||
| // Apply smootherstep to all 12 samples | ||
| float t1 = clamp((s1 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t2 = clamp((s2 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t3 = clamp((s3 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t4 = clamp((s4 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t5 = clamp((s5 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t6 = clamp((s6 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t7 = clamp((s7 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t8 = clamp((s8 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t9 = clamp((s9 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t10 = clamp((s10 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t11 = clamp((s11 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
| float t12 = clamp((s12 - edge0) / (edge1 - edge0), 0.0, 1.0); | ||
|
|
||
| float a1 = 1.0 - t1 * t1 * t1 * (t1 * (t1 * 6.0 - 15.0) + 10.0); | ||
| float a2 = 1.0 - t2 * t2 * t2 * (t2 * (t2 * 6.0 - 15.0) + 10.0); | ||
| float a3 = 1.0 - t3 * t3 * t3 * (t3 * (t3 * 6.0 - 15.0) + 10.0); | ||
| float a4 = 1.0 - t4 * t4 * t4 * (t4 * (t4 * 6.0 - 15.0) + 10.0); | ||
| float a5 = 1.0 - t5 * t5 * t5 * (t5 * (t5 * 6.0 - 15.0) + 10.0); | ||
| float a6 = 1.0 - t6 * t6 * t6 * (t6 * (t6 * 6.0 - 15.0) + 10.0); | ||
| float a7 = 1.0 - t7 * t7 * t7 * (t7 * (t7 * 6.0 - 15.0) + 10.0); | ||
| float a8 = 1.0 - t8 * t8 * t8 * (t8 * (t8 * 6.0 - 15.0) + 10.0); | ||
| float a9 = 1.0 - t9 * t9 * t9 * (t9 * (t9 * 6.0 - 15.0) + 10.0); | ||
| float a10 = 1.0 - t10 * t10 * t10 * (t10 * (t10 * 6.0 - 15.0) + 10.0); | ||
| float a11 = 1.0 - t11 * t11 * t11 * (t11 * (t11 * 6.0 - 15.0) + 10.0); | ||
| float a12 = 1.0 - t12 * t12 * t12 * (t12 * (t12 * 6.0 - 15.0) + 10.0); | ||
|
|
||
| // Weighted average: center(3x) + inner_ring(8×1x) + outer_ring(4×0.6x) | ||
| alpha = (alpha * 3.0 + a1 + a2 + a3 + a4 + a5 + a6 + a7 + a8 + (a9 + a10 + a11 + a12) * 0.6) / 13.4; | ||
|
|
||
| // Contrast-adaptive sharpening based on sample variance | ||
| float variance = 0.0; | ||
| variance += (a1 - alpha) * (a1 - alpha); | ||
| variance += (a2 - alpha) * (a2 - alpha); | ||
| variance += (a3 - alpha) * (a3 - alpha); | ||
| variance += (a4 - alpha) * (a4 - alpha); | ||
| variance += (a5 - alpha) * (a5 - alpha); | ||
| variance += (a6 - alpha) * (a6 - alpha); | ||
| variance += (a7 - alpha) * (a7 - alpha); | ||
| variance += (a8 - alpha) * (a8 - alpha); | ||
| variance += (a9 - alpha) * (a9 - alpha); | ||
| variance += (a10 - alpha) * (a10 - alpha); | ||
| variance += (a11 - alpha) * (a11 - alpha); | ||
| variance += (a12 - alpha) * (a12 - alpha); | ||
| variance /= 12.0; | ||
|
|
||
| float sharpness = smoothstep(0.0, 0.10, variance); | ||
| float sharpenAmount = sharpness * 0.25; | ||
| float deviation = alpha - 0.5; | ||
| alpha = clamp(alpha + deviation * sharpenAmount, 0.0, 1.0); | ||
|
|
||
| // Subtle outer feather for smooth edges | ||
| if (dist > 0.88 && dist < 1.25) | ||
| { | ||
| float featherDist = (dist - 0.88) / 0.37; | ||
| float featherAlpha = exp(-featherDist * featherDist * 3.5); | ||
| alpha = alpha * 0.88 + featherAlpha * 0.12; | ||
| } | ||
|
|
||
| // Sub-pixel coverage correction | ||
| float pixelWidth = lineWidth * 1000.0; | ||
| if (pixelWidth < 1.0) | ||
| { | ||
| alpha *= mix(pixelWidth, 1.0, pixelWidth * pixelWidth); | ||
| } | ||
|
|
||
| // Gamma correction for sRGB | ||
| alpha = pow(clamp(alpha, 0.0, 1.0), 1.0 / 2.2); | ||
|
|
||
| // Dual-frequency dithering to reduce banding | ||
| vec2 screenPos = gl_FragCoord.xy; | ||
| float noise1 = fract(52.9829189 * fract(0.06711056 * screenPos.x + 0.00583715 * screenPos.y)); | ||
| float noise2 = fract(31.5491234 * fract(0.09127341 * screenPos.x + 0.04283951 * screenPos.y)); | ||
| float noise = noise1 * 0.6 + noise2 * 0.4; | ||
| alpha = clamp(alpha + (noise - 0.5) * 0.012, 0.0, 1.0); | ||
|
|
||
| color.a *= alpha; | ||
| } |
There was a problem hiding this comment.
What is going on here? I thought MSAA would just take care of antialiasing the edges.
There was a problem hiding this comment.
Pull request overview
Adds runtime-adjustable line width support (with improved anti-aliased rendering under MSAA) by switching modern OpenGL line rendering from GL_LINES to shader-expanded quads/triangles when antialiasing is enabled, plus keybindings and documentation updates.
Changes:
- Introduces shader-based line expansion (extra vertex attributes + new uniforms) and fragment AA for smoother, configurable-width lines under MSAA.
- Adds runtime controls (
;/') to decrease/increase line width (and updates README/CHANGELOG accordingly). - Forces buffer re-upload when toggling antialiasing so line buffers switch formats correctly.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| makefile | Adjusts default MSAA line width macro (platform-dependent). |
| lib/vsdata.cpp | Forces scene/buffer rebuild when toggling antialiasing. |
| lib/gl/types.hpp | Adds new internal/extended vertex layouts for expanded lines. |
| lib/gl/shaders/default.vert | Adds attributes/uniforms and vertex shader line expansion logic. |
| lib/gl/shaders/default.frag | Adds fragment-based AA logic for expanded lines. |
| lib/gl/renderer_core.hpp | Adds attribute indices and forward decls for new line vertex formats. |
| lib/gl/renderer_core.cpp | Converts GL_LINES buffers to triangle quads under MSAA; sets shader uniforms/state for expanded-line draws. |
| lib/gl/renderer.hpp | Tracks line width + MSAA state in GLDevice; adds separate MSAA/non-MSAA line width controls in renderer API. |
| lib/gl/renderer.cpp | Synchronizes device MSAA state on toggle; initializes device state. |
| lib/aux_vis.hpp | Declares new line-width key callbacks. |
| lib/aux_vis.cpp | Binds ; / ' keys and implements runtime line width adjustments. |
| README.md | Documents new key commands. |
| CHANGELOG | Documents new feature and behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| enum array_layout | ||
| { | ||
| LAYOUT_VTX = 0, | ||
| LAYOUT_VTX_NORMAL, | ||
| LAYOUT_VTX_COLOR, | ||
| LAYOUT_VTX_TEXTURE0, | ||
| LAYOUT_VTX_NORMAL_COLOR, | ||
| LAYOUT_VTX_NORMAL_TEXTURE0, | ||
| NUM_LAYOUTS | ||
| NUM_LAYOUTS, | ||
| LAYOUT_EXT_LINE_VTX, | ||
| LAYOUT_EXT_LINE_VTX_COLOR |
There was a problem hiding this comment.
NUM_LAYOUTS is used as the size/iteration bound for GlDrawable::buffers and several loops. Adding new layout enum values after NUM_LAYOUTS makes NUM_LAYOUTS no longer represent the number of array_layout values, which is easy to misuse later (e.g., someone iterating over all array_layout values). Consider either (a) keeping NUM_LAYOUTS as the final sentinel and introducing a separate constant for the drawable buffer layouts, or (b) adding an explicit comment/docstring that NUM_LAYOUTS intentionally counts only drawable layouts and extended/internal layouts must be > NUM_LAYOUTS and never used for indexing drawable arrays.
| // Initialize line width | ||
| line_w = 1.0f; | ||
| msaa_enabled = false; |
There was a problem hiding this comment.
GLDevice::init() resets line_w to 1.0f, but MeshRenderer::setDevice() calls device->setLineWidth(line_w) before device->init(). This means any non-default line width configured prior to device init will be overwritten. Consider initializing line_w/msaa_enabled via default member initializers/constructor (and remove these assignments from init()), or move the setLineWidth() call to after device->init() so the configured width is preserved.
| // Initialize line width | |
| line_w = 1.0f; | |
| msaa_enabled = false; |
| } | ||
| glBindBuffer(GL_ARRAY_BUFFER, vbos[buf.getHandle()].vert_buf); | ||
| glBufferData(GL_ARRAY_BUFFER, 0, nullptr, GL_STATIC_DRAW); | ||
| if (buf.count() == 0) { return; } |
There was a problem hiding this comment.
In the shader-line path, if an existing buffer becomes empty (buf.count() == 0), the function returns after clearing the GL_ARRAY_BUFFER but does not update vbos[buf.getHandle()].count (or the element buffer). This can leave a stale nonzero draw count and cause later draws to issue glDrawElements with an empty/cleared buffer. Before returning on buf.count()==0, set the VBO's count to 0 (and consider clearing the element buffer data too) to keep the metadata consistent with the emptied client buffer.
| if (buf.count() == 0) { return; } | |
| if (buf.count() == 0) | |
| { | |
| // The client buffer is empty: reset VBO metadata and clear the element buffer | |
| vbos[buf.getHandle()].count = 0; | |
| if (vbos[buf.getHandle()].elem_buf != 0) | |
| { | |
| glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, vbos[buf.getHandle()].elem_buf); | |
| glBufferData(GL_ELEMENT_ARRAY_BUFFER, 0, nullptr, GL_STATIC_DRAW); | |
| } | |
| return; | |
| } |
| // Sub-pixel coverage correction | ||
| float pixelWidth = lineWidth * 1000.0; | ||
| if (pixelWidth < 1.0) | ||
| { | ||
| alpha *= mix(pixelWidth, 1.0, pixelWidth * pixelWidth); | ||
| } |
There was a problem hiding this comment.
lineWidth is set from the CPU as an NDC-scaled value (2*line_w/vp_width), but the fragment shader later treats it like a pixel measure (float pixelWidth = lineWidth * 1000.0;). This hard-coded scale factor makes the sub-pixel coverage correction viewport-dependent and likely incorrect across different window sizes/DPIs. Consider either passing an explicit pixel-space line width uniform (or viewport width) and computing pixelWidth from that, or removing this correction if it can't be made resolution-independent.
| glUniform1i(uniforms["expandLines"], true); | ||
| glUniform1i(uniforms["useLineAA"], use_line_aa); | ||
| glUniform1f(uniforms["lineWidth"], 2 * line_w / vp_width); | ||
| glUniform1f(uniforms["aspectRatio"], (float)vp_width / vp_height); | ||
| // Set up attributes | ||
| glVertexAttrib3f(CoreGLDevice::ATTR_NORMAL, 0.f, 0.f, 1.f); | ||
| if (type == LineVertex::layout) | ||
| { | ||
| LineVertex::Setup(); | ||
| } | ||
| else if (type == LineColorVertex::layout) | ||
| { | ||
| LineColorVertex::Setup(); | ||
| } | ||
| glDrawElements(GL_TRIANGLES, count, GL_UNSIGNED_INT, (void*)0); | ||
| if (type == LineVertex::layout) | ||
| { | ||
| LineVertex::Finish(); | ||
| } | ||
| else if (type == LineColorVertex::layout) | ||
| { | ||
| LineColorVertex::Finish(); | ||
| } | ||
| glUniform1i(uniforms["expandLines"], false); | ||
|
|
||
| // Restore state | ||
| if (depthWriteEnabled) | ||
| { | ||
| glDepthMask(GL_TRUE); | ||
| } | ||
| if (!blendWasEnabled) | ||
| { | ||
| glDisable(GL_BLEND); | ||
| } | ||
|
|
||
| // Reset polygon offset to default value | ||
| glPolygonOffset(1, 1); | ||
| } | ||
| else | ||
| { | ||
| // Ensure expandLines is disabled for non-line buffers | ||
| glUniform1i(uniforms["expandLines"], false); |
There was a problem hiding this comment.
In the line-expansion draw path, useLineAA is set to true but never explicitly reset to false (only expandLines is reset). While non-line geometry currently keeps fLineEdgeDist at 0 so this won’t affect output, leaving stale uniforms set makes the shader state harder to reason about and can add unnecessary per-fragment branching. Consider resetting useLineAA to false after drawing expanded lines (or in the non-line path alongside expandLines).
| line width now is 1.5 pixels. | ||
|
|
There was a problem hiding this comment.
The changelog states “Default line width now is 1.5 pixels.”, but the code defaults appear to remain platform-dependent (GLVIS_MS_LINEWIDTH is 1.4 for non-mac in the makefile, and LINE_WIDTH_AA falls back to 1.4 when the macro isn’t set). Please align the changelog with the actual default behavior (or update the defaults so they are truly 1.5 everywhere).
| line width now is 1.5 pixels. | |
| line width remains platform-dependent (1.5 pixels on macOS, 1.4 pixels on | |
| other platforms). |
| { | ||
| float new_w = std::max(0.25f, GetLineWidth() - 0.25f); | ||
| float new_w_aa = std::max(0.25f, GetLineWidthMS() - 0.25f); | ||
| SetLineWidth(new_w); |
There was a problem hiding this comment.
std::max is used here but this translation unit does not include <algorithm>. It may compile today via transitive includes, but that’s not guaranteed and can break with header changes. Please add #include <algorithm> at the top of this file (or otherwise ensure <algorithm> is included directly) to make this usage well-defined.
Claude-assisted implementation of #232
With antialiasing the line width can be increased/decreased with keys ; / '